-
Notifications
You must be signed in to change notification settings - Fork 0
#149 - Merge labels from PR and the connected issue #159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#149 - Merge labels from PR and the connected issue #159
Conversation
- Labels are now holded on record itself as set of strings.
Caution Review failedThe pull request is closed. WalkthroughConsolidated label handling into the base Record class using an internal set, removed per-record public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Builder as ReleaseNotesBuilder
participant Factory as RecordFactory
participant Issue as Issue (GitHub)
participant PR as PullRequest (GitHub)
participant IssueRec as IssueRecord
participant PRRec as PullRequestRecord
User->>Builder: build()
Builder->>Factory: create records from repo data
Factory->>Issue: call get_labels()
Factory->>PR: call get_labels()
Factory->>IssueRec: IssueRecord.__init__(_labels ← issue label names)
Factory->>PRRec: PullRequestRecord.__init__(_labels ← pr label names)
Builder->>IssueRec: register(PRRec)
Note right of IssueRec: IssueRec._labels ← IssueRec._labels ∪ PRRec._labels
Builder->>Builder: categorize by IssueRec.labels (list(_labels))
Builder-->>User: release notes grouped by merged labels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/release_notes/test_release_notes_builder.py (1)
552-553
: Replace stale.issue.labels
mutation with_labels
In tests/release_notes/test_release_notes_builder.py line 552, update:- rec.issue.labels.append(MockLabel("enhancement")) + rec._labels.add("enhancement")No other
.issue.labels
or.pull_request.labels
mutations remain in the tests.
🧹 Nitpick comments (2)
release_notes_generator/model/issue_record.py (1)
30-31
: Good: Issue labels cached and PR labels merged into the same set (meets #149). Consider using get_labels() and optional normalization.This implements the objective to merge PR and Issue labels. For SDK compatibility, mirror the
get_labels()
approach here as well.Proposed adjustments:
- self._labels = {label.name for label in self._issue.labels} + if hasattr(self._issue, "get_labels"): + self._labels = {label.name for label in self._issue.get_labels()} + else: + self._labels = {label.name for label in getattr(self._issue, "labels", [])}- self._labels.update({label.name for label in pull.labels}) + if hasattr(pull, "get_labels"): + self._labels.update({label.name for label in pull.get_labels()}) + else: + self._labels.update({label.name for label in getattr(pull, "labels", [])})Optionally, normalize names (e.g.,
.lower()
) to avoid case-sensitivity mismatches with chapter label keys. I can wire that if desired.Also applies to: 162-162
tests/release_notes/test_release_notes_builder.py (1)
714-714
: Tests poke private_labels
; prefer updating the existing set rather than rebinding itDirectly assigning a new set replaces the object; using
update()
avoids surprises and reads clearer. Longer term, consider a small helper (e.g.,rec.add_labels([...])
) to avoid touching a private field from tests.Apply:
- rec._labels = {"bug", "breaking-changes"} + rec._labels.update({"bug", "breaking-changes"})- rec._labels = {"label1", "label2"} + rec._labels.update({"label1", "label2"})- rec._labels = {"bug", "enhancement"} + rec._labels.update({"bug", "enhancement"})I can add a tiny test helper or a public method to seed labels if you want to avoid accessing
_labels
directly.Also applies to: 733-733, 828-828
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
main.py
(0 hunks)release_notes_generator/model/commit_record.py
(0 hunks)release_notes_generator/model/issue_record.py
(2 hunks)release_notes_generator/model/pull_request_record.py
(1 hunks)release_notes_generator/model/record.py
(2 hunks)tests/release_notes/test_release_notes_builder.py
(3 hunks)
💤 Files with no reviewable changes (2)
- release_notes_generator/model/commit_record.py
- main.py
🧰 Additional context used
🧬 Code graph analysis (2)
release_notes_generator/model/pull_request_record.py (1)
tests/release_notes/model/test_record.py (1)
labels
(41-42)
release_notes_generator/model/issue_record.py (1)
tests/release_notes/model/test_record.py (1)
labels
(41-42)
🔇 Additional comments (1)
release_notes_generator/model/record.py (1)
41-41
: Good: Centralized label storage on the base classCaching labels as a set here simplifies merging and deduplication.
self._labels = {label.name for label in self._pull_request.labels} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Initialize labels via PyGithub API method for compatibility and determinism
Prefer get_labels()
over the .labels
attribute to avoid SDK differences and lazy-loading surprises. This also aligns with how Issue
labels are typically fetched.
Proposed change:
- self._labels = {label.name for label in self._pull_request.labels}
+ # Prefer the API method when available; fall back to attribute for mocks/tests
+ if hasattr(self._pull_request, "get_labels"):
+ self._labels = {label.name for label in self._pull_request.get_labels()}
+ else:
+ self._labels = {label.name for label in getattr(self._pull_request, "labels", [])}
If you want me to normalize casing (e.g., .lower()
) for safer chapter matching, I can add that as well.
To confirm SDK behavior, please check:
🌐 Web query:
Does PyGithub PullRequest expose a `.labels` attribute or should labels be fetched via `get_labels()`?
💡 Result:
Use get_labels(). The official PyGithub PullRequest object exposes a get_labels() method (returns a PaginatedList of Label) and label-modifying methods (add_to_labels, remove_from_labels, set_labels, etc.). While some REPL/examples may show a .labels attribute in certain contexts, the documented/official way is pr.get_labels(). [1][2]
Sources:
- PyGithub PullRequest docs (get_labels, add_to_labels, etc.). [1]
- GitHub issue/example showing .labels in REPL but confirming get_labels() usage. [2]
Use get_labels()
for deterministic label retrieval
Replace the .labels
attribute with the documented get_labels()
API, falling back to the attribute for mocks/tests:
- self._labels = {label.name for label in self._pull_request.labels}
+ # Prefer the API method; fallback for mocks/tests
+ if hasattr(self._pull_request, "get_labels"):
+ self._labels = {label.name for label in self._pull_request.get_labels()}
+ else:
+ self._labels = {label.name for label in getattr(self._pull_request, "labels", [])}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
self._labels = {label.name for label in self._pull_request.labels} | |
# Prefer the API method; fallback for mocks/tests | |
if hasattr(self._pull_request, "get_labels"): | |
self._labels = {label.name for label in self._pull_request.get_labels()} | |
else: | |
self._labels = {label.name for label in getattr(self._pull_request, "labels", [])} |
🤖 Prompt for AI Agents
In release_notes_generator/model/pull_request_record.py around lines 28-29,
replace direct access to self._pull_request.labels with the documented API: call
self._pull_request.get_labels() to retrieve labels, and fall back to the .labels
attribute for mocks/tests if get_labels is not present; then build the set with
something like {label.name for label in labels_iterable} (ensuring you treat the
result as an iterable) so label retrieval remains deterministic and compatible
with both real objects and test doubles.
@property | ||
def labels(self) -> list[str]: | ||
""" | ||
Gets the labels of the record. | ||
Returns: | ||
list[str]: A list of labels associated with the record. | ||
""" | ||
return list(self._labels) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Make label exposure deterministic to prevent flaky ordering
Returning a list from a set yields non-deterministic order; sorting avoids flaky tests and unstable output.
Apply:
- return list(self._labels)
+ return sorted(self._labels)
Optionally, if you’d like a public helper to simplify tests and avoid touching _labels
, consider:
# outside diff – add to Record
def add_labels(self, labels: list[str]) -> None:
self._labels.update(labels)
🤖 Prompt for AI Agents
In release_notes_generator/model/record.py around lines 59 to 67, the labels
property returns list(self._labels) which yields non-deterministic ordering;
change the property to return a consistently ordered list by converting the set
to a sorted list (e.g., return sorted(self._labels)); optionally add a public
helper method add_labels(self, labels: list[str]) that updates the internal set
via self._labels.update(labels) to simplify tests without touching the private
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miroslavpojer, understood! I'll defer to your judgment on this. The current implementation works fine for your use case.
🐰
╭─────╮
│ ◕ ◕ │
│ ‿ │
╰─────╯
- Fixed unit tests.
- Fixed unit tests. - Partial fix of pylint messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
DEVELOPER.md (2)
52-52
: Fix wrong module path in example.
Project package uses underscores, not hyphens.-pylint release-notes-generator/generator.py +pylint release_notes_generator/generator.py
99-120
: Typo: “my[py]” → “mypy” (multiple lines).
Several occurrences will confuse readers and search.-This project uses the [my[py]](https://mypy.readthedocs.io/en/stable/) tool, a static type checker for Python. +This project uses the [mypy](https://mypy.readthedocs.io/en/stable/) tool, a static type checker for Python. @@ -my[py] configuration is in `pyproject.toml` file. +mypy configuration is in `pyproject.toml` file. @@ -Follow these steps to format your code with my[py] locally: +Follow these steps to run mypy locally: @@ -### Run my[py] +### Run mypy @@ -Run my[py] on all files in the project. +Run mypy on all files in the project. @@ - mypy . +mypy . @@ -To run my[py] check on a specific file, follow the pattern `mypy <path_to_file>/<name_of_file>.py --check-untyped-defs`. +To run mypy on a specific file, use `mypy <path_to_file>/<name_of_file>.py --check-untyped-defs`.tests/release_notes/test_record_factory.py (2)
212-213
: Patch target is wrong; you’re not patching the symbol used by RecordFactory.
RecordFactory imports ActionInputs fromrelease_notes_generator.action_inputs
. Patch that symbol, or patch the bound name in record_factory.-mocker.patch("release_notes_generator.builder.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes"]) +mocker.patch("release_notes_generator.record.record_factory.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes"])Alternatively:
-mocker.patch("release_notes_generator.builder.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes"]) +mocker.patch("release_notes_generator.action_inputs.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes"])
377-378
: Same incorrect patch target here.
Update to patch the symbol actually referenced by record_factory.-mocker.patch("release_notes_generator.builder.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes", "another-skip-label"]) +mocker.patch("release_notes_generator.record.record_factory.ActionInputs.get_skip_release_notes_labels", return_value=["skip-release-notes", "another-skip-label"])
🧹 Nitpick comments (4)
DEVELOPER.md (1)
34-36
: Good: standardized venv + pip upgrade. Align the Black section too.
Black section still usesvenv/
and lacks the pip upgrade. Unify to.venv
for consistency.Apply:
@@ -```shell -python3 -m venv venv -source venv/bin/activate -pip install -r requirements.txt -``` +```shell +python3 -m venv .venv +source .venv/bin/activate +pip install --upgrade pip +pip install -r requirements.txt +```release_notes_generator/record/record_factory.py (1)
59-60
: Remove unused parameter skip_rec in nested function.
It’s never used; simplify signature and call.- def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: + def register_pull_request(pull: PullRequest) -> None: @@ - register_pull_request(pull, skip_record) + register_pull_request(pull)Also applies to: 109-109
tests/conftest.py (2)
438-441
: Remove stray TODO in fixture name comment.
Avoid stale comments in tests.- rec = IssueRecord(issue=request.getfixturevalue("mock_issue_closed_i1_bug")) # TODO - renamed from mock_issue_closed_i2_bug + rec = IssueRecord(issue=request.getfixturevalue("mock_issue_closed_i1_bug"))
136-141
: Deduplicate label mock creation across fixtures.
Introduce a tiny helper to cut repetition.Add once near the top of this file:
def make_labels(mocker, *names): labs = [] for n in names: l = mocker.Mock() l.name = n labs.append(l) return labsThen refactor calls, e.g.:
- label1 = mocker.Mock(spec=MockLabel) - label1.name = "label1" - label2 = mocker.Mock(spec=MockLabel) - label2.name = "label2" - issue.get_labels.return_value = [label1, label2] + issue.get_labels.return_value = make_labels(mocker, "label1", "label2")Also applies to: 154-158, 169-174, 201-206
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
DEVELOPER.md
(1 hunks)release_notes_generator/model/issue_record.py
(2 hunks)release_notes_generator/model/pull_request_record.py
(1 hunks)release_notes_generator/record/record_factory.py
(2 hunks)requirements.txt
(1 hunks)tests/conftest.py
(10 hunks)tests/release_notes/test_record_factory.py
(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- release_notes_generator/model/pull_request_record.py
- release_notes_generator/model/issue_record.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/conftest.py (2)
release_notes_generator/model/issue_record.py (2)
issue
(58-63)IssueRecord
(16-287)tests/release_notes/test_release_notes_builder.py (1)
MockLabel
(75-77)
🔇 Additional comments (4)
requirements.txt (1)
1-7
: Confirm runtime compatibility after version bumps (Black 25.x, PyGithub 2.x)
No CI workflows found—manually verify CI and local dev environments use a Python version compatible with these pins. Ensurepython_requires
is declared in setup.py/setup.cfg/pyproject.toml. Double-check any PyGithub label API usage for breaking changes.tests/conftest.py (2)
132-141
: LGTM: fixtures now set explicit issue fields and use get_labels().
Good move toward the production path and clearer assertions.
224-228
: LGTM: PR fixtures switched to get_labels().
Keeps tests aligned with the PyGithub 2.x API usage.tests/release_notes/test_record_factory.py (1)
48-49
: LGTM: migrated to get_labels() for PRs.
Matches production code and PyGithub 2.x.
@@ -98,7 +98,7 @@ def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: | |||
|
|||
logger.debug("Registering pull requests to records...") | |||
for pull in data.pull_requests: | |||
pull_labels = [label.name for label in pull.labels] | |||
pull_labels = [label.name for label in pull.get_labels()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Wrap API call to get_labels with rate-limiter.
Direct call may hit rate limits; use the existing safe_call wrapper.
- pull_labels = [label.name for label in pull.get_labels()]
+ pull_labels = [label.name for label in safe_call(pull.get_labels)()]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
pull_labels = [label.name for label in pull.get_labels()] | |
pull_labels = [label.name for label in safe_call(pull.get_labels)()] |
🤖 Prompt for AI Agents
In release_notes_generator/record/record_factory.py around line 101, the direct
call pull.get_labels() can hit API rate limits; replace it by invoking the
existing safe_call wrapper to perform the API call (e.g., call safe_call with
pull.get_labels or a lambda that calls it), assign the returned value to a
variable, and fall back to an empty list if safe_call returns None or raises;
ensure you import/Reference the existing safe_call and use its return value to
build pull_labels = [label.name for label in labels].
issue_labels = [label.name for label in i.get_labels()] | ||
skip_record = any(item in issue_labels for item in ActionInputs.get_skip_release_notes_labels()) | ||
records[i.number] = IssueRecord(issue=i, skip=skip_record) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Same: wrap issue.get_labels with rate-limiter.
Aligns with other GitHub API calls here.
- issue_labels = [label.name for label in i.get_labels()]
+ issue_labels = [label.name for label in safe_call(i.get_labels)()]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
issue_labels = [label.name for label in i.get_labels()] | |
skip_record = any(item in issue_labels for item in ActionInputs.get_skip_release_notes_labels()) | |
records[i.number] = IssueRecord(issue=i, skip=skip_record) | |
issue_labels = [label.name for label in safe_call(i.get_labels)()] | |
skip_record = any(item in issue_labels for item in ActionInputs.get_skip_release_notes_labels()) | |
records[i.number] = IssueRecord(issue=i, skip=skip_record) |
🤖 Prompt for AI Agents
In release_notes_generator/record/record_factory.py around lines 161 to 164, the
direct call to i.get_labels() should be invoked through the existing GitHub API
rate-limiter like the other calls in this file; replace the direct list
comprehension with a rate-limited call that fetches labels (e.g., call the
module/class rate-limiter wrapper used elsewhere to execute i.get_labels()),
then build issue_labels from that returned value and proceed to compute
skip_record and IssueRecord as before; ensure you use the same rate-limiter
instance/name already used in this file so imports/variables remain consistent.
Release Notes:
Fixes #149
Summary by CodeRabbit